APIGW Normalize Status Conditions by jm96441n · Pull Request #16994 · hashicorp/consul

jm96441n · 2023-04-13T02:22:54Z

Description

We want to normalize status conditions for gateways and routes to more closely match the k8s spec. This PR also includes validations for combinations of status/type/reason. We deviate from the spec in a few places on routes and this is called out in the code.

Testing & Reproduction steps

Run the tests

Links

k8s spec
k8s shared types

PR Checklist

updated test coverage
external facing docs updated
appropriate backport labels added
not a security concern

agent/consul/fsm_data_store.go

jm96441n · 2023-04-13T02:23:55Z

agent/consul/gateways/controller_gateways.go


-// gatewayConditionGenerator is a simple struct used for isolating
-// the status conditions that we generate for our components
-type gatewayConditionGenerator struct {


this struct was no longer needed because we encapsulated creating the conditions in the structs package along with validations

jm96441n · 2023-04-13T02:24:40Z

agent/structs/config_entry_status.go

+	if err := checkRouteConditionReason(name, status, reason); err != nil {
+		// note we panic here because an invalid combination is a programmer error
+		// this  should never actually be hit
+		panic(err)


we panic here and in the gateway constructor because an error here is 100% programmer error

jm96441n · 2023-04-13T15:11:51Z

agent/structs/config_entry_status_test.go

+	}
+}
+
+func TestNewGatewayInvalidCombinationsCausePanic(t *testing.T) {


I was debating making this more of a property based test and randomizing the status/reason/type used but figured that was a bit overkill for this set of tests

agent/consul/gateways/controller_gateways.go

jm96441n · 2023-04-13T17:59:21Z

agent/consul/gateways/controller_gateways.go

-		now: pointerTo(time.Now().UTC()),
-	}
+// gatewayAccepted marks the APIGateway as valid.
+func gatewayAccepted() structs.Condition {


so I reorganized the order of these methods to group all the condition types together, I also debated moving these into the config_entry_status file since they're pretty much just using all types/values from that package, curious if anyone has any opinions on it

jm96441n · 2023-04-13T17:59:48Z

agent/consul/gateways/controller_gateways.go

-		Message:            "gateway references invalid certificates",
-		LastTransitionTime: g.now,
-	}
+func invalidCertificates() structs.Condition {


now this and invalidCertificate fail for the same Condition/Reason but have different messages

so @andrewstucki this is now the only one (along with invalidCertificate ) to have a different type/reason (so ResolvedRefs type and InvalidCertifcateRef for the reason vs Accepted type and InvalidCertificate for the reason). Do you have an opinion on whether this makes sense or should be reverted?

So, I'm thinking that it makes sense to move invalidCertificate to a ResolvedRefs status along with keeping it scoped to an individual listener. I'd be ok potentially dropping this entire status, but maybe in a separate PR after discussing a bit more. Specifically here's the issue:

I believe we want Accepted to be a rolled up condition as to whether a Gateway/Route is invalid or not

We actually have a check that a Gateway has Accepted == True in order for it to actually bind any routes (meaning changing this will change this behavior):

consul/agent/consul/gateways/controller_gateways.go

Lines 673 to 682 in a3bf847

// check to make sure we're not binding to an invalid gateway

if !g.Gateway.Status.MatchesConditionStatus(gatewayAccepted()) {

return false, fmt.Errorf("failed to bind route to gateway %s: gateway has not been accepted", g.Gateway.Name)

}

// check to make sure we're not binding to an invalid route

status := route.GetStatus()

if !status.MatchesConditionStatus(routeAccepted()) {

return false, fmt.Errorf("failed to bind route to gateway %s: route has not been accepted", g.Gateway.Name)

}

What this currently means is that if any of our certificate refs are invalid across any listener, we actually say that the Gateway can't bind a route, that's probably wrong and should probably change and be scoped to a given listener (which the method that this check is contained in actually has access to).

If we want to fix it, rather than mark the entire Gateway as not Accepted, we could potentially mark each Listener individually as Accepted and scope the above binding check to a listener. That said, that would potentially defeat the purpose of considering Accepted as a rolled-up status since I can't see how else an entire Gateway would ever be Accepted == False.

So, the TLDR; on my initial thoughts are:

keep this Accepted, switch invalidCertificate to ResolvedRefs, at least for this PR

but I'm definitely open to discussion.

@mikemorris any thoughts?

that makes sense to me, if anything we can make changes in a future PR once we get a better feel for how this should look

andrewstucki

So, I like the better strong typing, but I think we need to clean up and have only a subset of these specified. We don't need to dupe the entire upstream spec because a number of these conditions/reasons are things that we shouldn't ever allow to be written in the first place.

agent/structs/config_entry_status.go

agent/consul/gateways/controller_gateways.go

agent/structs/config_entry_status.go

validating combinations, added dummy code for fsm store

jm96441n · 2023-04-18T18:24:00Z

api/go.mod

 	github.com/hashicorp/serf v0.10.1
 	github.com/mitchellh/mapstructure v1.4.1
 	github.com/stretchr/testify v1.7.0
+	golang.org/x/exp v0.0.0-20230321023759-10a507213a29


so running go mod tidy here to get exp slices package caused a cascading need of changes to other go.mod files that import this module which is why this change seems larger than it is

andrewstucki

LGTM, thanks for the changes earlier and I'm glad we have some better type-safety here now!

* normalize status conditions for gateways and routes * Added tests for checking condition status and panic conditions for validating combinations, added dummy code for fsm store * get rid of unneeded gateway condition generator struct * Remove unused file * run go mod tidy * Update tests, add conflicted gateway status * put back removed status for test * Fix linting violation, remove custom conflicted status * Update fsm commands oss * Fix incorrect combination of type/condition/status * cleaning up from PR review * Change "invalidCertificate" to be of accepted status * Move status condition enums into api package * Update gateways controller and generated code * Update conditions in fsm oss tests * run go mod tidy on consul-container module to fix linting * Fix type for gateway endpoint test * go mod tidy from changes to api * go mod tidy on troubleshoot * Fix route conflicted reason * fix route conflict reason rename * Fix text for gateway conflicted status * Add valid certificate ref condition setting * Revert change to resolved refs to be handled in future PR

…7844) * APIGW Normalize Status Conditions (#16994) * normalize status conditions for gateways and routes * Added tests for checking condition status and panic conditions for validating combinations, added dummy code for fsm store * get rid of unneeded gateway condition generator struct * Remove unused file * run go mod tidy * Update tests, add conflicted gateway status * put back removed status for test * Fix linting violation, remove custom conflicted status * Update fsm commands oss * Fix incorrect combination of type/condition/status * cleaning up from PR review * Change "invalidCertificate" to be of accepted status * Move status condition enums into api package * Update gateways controller and generated code * Update conditions in fsm oss tests * run go mod tidy on consul-container module to fix linting * Fix type for gateway endpoint test * go mod tidy from changes to api * go mod tidy on troubleshoot * Fix route conflicted reason * fix route conflict reason rename * Fix text for gateway conflicted status * Add valid certificate ref condition setting * Revert change to resolved refs to be handled in future PR * Resolve sneaky merge conflicts --------- Co-authored-by: John Maguire <john.maguire@hashicorp.com> Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

jm96441n added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-docs PR does not include docs and should not trigger reminder for cherrypicking them. backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. labels Apr 13, 2023

jm96441n commented Apr 13, 2023

View reviewed changes

agent/consul/fsm_data_store.go Outdated Show resolved Hide resolved

jm96441n commented Apr 13, 2023

View reviewed changes

jm96441n marked this pull request as draft April 13, 2023 02:25

vercel bot deployed to Preview – consul April 13, 2023 02:31 View deployment

jm96441n commented Apr 13, 2023

View reviewed changes

agent/consul/gateways/controller_gateways.go Outdated Show resolved Hide resolved

jm96441n marked this pull request as ready for review April 13, 2023 17:54

jm96441n commented Apr 13, 2023

View reviewed changes

andrewstucki suggested changes Apr 13, 2023

View reviewed changes

andrewstucki reviewed Apr 13, 2023

View reviewed changes

agent/consul/gateways/controller_gateways.go Outdated Show resolved Hide resolved

jm96441n commented Apr 17, 2023

View reviewed changes

agent/structs/config_entry_status.go Outdated Show resolved Hide resolved

vercel bot deployed to Preview – consul April 17, 2023 17:28 View deployment

vercel bot deployed to Preview – consul April 18, 2023 15:49 View deployment

jm96441n added 12 commits April 18, 2023 13:54

normalize status conditions for gateways and routes

c777004

Added tests for checking condition status and panic conditions for

eb62bef

validating combinations, added dummy code for fsm store

get rid of unneeded gateway condition generator struct

bb50b96

Remove unused file

2659a06

run go mod tidy

b84cd21

Update tests, add conflicted gateway status

740a4e5

put back removed status for test

8a8662b

Fix linting violation, remove custom conflicted status

8864986

Update fsm commands oss

01a39fb

Fix incorrect combination of type/condition/status

b9f369a

cleaning up from PR review

1b6a5b6

Change "invalidCertificate" to be of accepted status

7129105

jm96441n added 5 commits April 18, 2023 13:55

Move status condition enums into api package

c6f1615

Update gateways controller and generated code

fa978fd

Update conditions in fsm oss tests

11d59a3

run go mod tidy on consul-container module to fix linting

d26c63a

Fix type for gateway endpoint test

519acc4

jm96441n force-pushed the jm96441n/normalize-status-conditions branch from f1bfee0 to 519acc4 Compare April 18, 2023 17:55

jm96441n added 2 commits April 18, 2023 14:07

go mod tidy from changes to api

2da0633

go mod tidy on troubleshoot

c1686a9

jm96441n commented Apr 18, 2023

View reviewed changes

jm96441n added 5 commits April 18, 2023 14:43

Fix route conflicted reason

3c7df32

fix route conflict reason rename

0b3ec3a

Fix text for gateway conflicted status

9277645

Merge branch 'main' into jm96441n/normalize-status-conditions

6dc16c9

Merge branch 'main' into jm96441n/normalize-status-conditions

1be474f

vercel bot deployed to Preview – consul-ui-staging April 19, 2023 22:05 View deployment

jm96441n added 2 commits April 20, 2023 09:38

Merge branch 'main' into jm96441n/normalize-status-conditions

67fb620

Merge branch 'main' into jm96441n/normalize-status-conditions

eb3b997

vercel bot deployed to Preview – consul April 24, 2023 17:09 View deployment

andrewstucki approved these changes Apr 24, 2023

View reviewed changes

jm96441n added 2 commits April 24, 2023 15:25

Add valid certificate ref condition setting

d83a09c

Revert change to resolved refs to be handled in future PR

f1fa81a

jm96441n merged commit e47f321 into main Apr 24, 2023

jm96441n deleted the jm96441n/normalize-status-conditions branch April 24, 2023 20:22

mikemorris mentioned this pull request Apr 26, 2023

gateways: add Gateway and Route config entry status condition types and helpers #16141

Closed

3 tasks

nathancoleman added backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. and removed backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. labels Jun 22, 2023

hc-github-team-consul-core mentioned this pull request Jun 22, 2023

Backport of APIGW Normalize Status Conditions into release/1.15.x #17844

Merged

4 tasks

	// check to make sure we're not binding to an invalid gateway
	if !g.Gateway.Status.MatchesConditionStatus(gatewayAccepted()) {
	return false, fmt.Errorf("failed to bind route to gateway %s: gateway has not been accepted", g.Gateway.Name)
	}

	// check to make sure we're not binding to an invalid route
	status := route.GetStatus()
	if !status.MatchesConditionStatus(routeAccepted()) {
	return false, fmt.Errorf("failed to bind route to gateway %s: route has not been accepted", g.Gateway.Name)
	}

Conversation

jm96441n commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing & Reproduction steps

Links

PR Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewstucki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewstucki left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jm96441n commented Apr 13, 2023 •

edited

Loading